Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std: Stabilize a number of new fs features #25844

Merged
merged 1 commit into from Jun 12, 2015

Conversation

alexcrichton
Copy link
Member

This commit stabilizes the following APIs, slating them all to be cherry-picked
into the 1.1 release.

  • fs::FileType (and transitively the derived trait implementations)
  • fs::Metadata::file_type
  • fs::FileType::is_dir
  • fs::FileType::is_file
  • fs::FileType::is_symlink
  • fs::DirEntry::metadata
  • fs::DirEntry::file_type
  • fs::DirEntry::file_name
  • fs::set_permissions
  • fs::symlink_metadata
  • os::raw::{self, *}
  • os::{android, bitrig, linux, ...}::raw::{self, *}
  • os::{android, bitrig, linux, ...}::fs::MetadataExt
  • os::{android, bitrig, linux, ...}::fs::MetadataExt::as_raw_stat
  • os::unix::fs::PermissionsExt
  • os::unix::fs::PermissionsExt::mode
  • os::unix::fs::PermissionsExt::set_mode
  • os::unix::fs::PermissionsExt::from_mode
  • os::unix::fs::OpenOptionsExt
  • os::unix::fs::OpenOptionsExt::mode
  • os::unix::fs::DirEntryExt
  • os::unix::fs::DirEntryExt::ino
  • os::windows::fs::MetadataExt
  • os::windows::fs::MetadataExt::file_attributes
  • os::windows::fs::MetadataExt::creation_time
  • os::windows::fs::MetadataExt::last_access_time
  • os::windows::fs::MetadataExt::last_write_time
  • os::windows::fs::MetadataExt::file_size

The os::unix::fs::Metadata structure was also removed entirely, moving all of
its associated methods into the os::unix::fs::MetadataExt trait instead. The
methods are all marked as #[stable] still.

As some minor cleanup, some deprecated and unstable fs apis were also removed:

  • File::path
  • Metadata::accessed
  • Metadata::modified

Features that were explicitly left unstable include:

  • fs::WalkDir - the semantics of this were not considered in the recent fs
    expansion RFC.
  • fs::DirBuilder - it's still not 100% clear if the naming is right here and if
    the set of functionality exposed is appropriate.
  • fs::canonicalize - the implementation on Windows here is specifically in
    question as it always returns a verbatim path. Additionally the Unix
    implementation is susceptible to buffer overflows on long paths unfortunately.
  • fs::PathExt - as this is just a convenience trait, it is not stabilized at
    this time.
  • fs::set_file_times - this funciton is still waiting on a time abstraction.

@rust-highfive
Copy link
Collaborator

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @aturon

cc @rust-lang/libs

@rust-highfive rust-highfive assigned aturon and unassigned huonw May 27, 2015
@alexcrichton alexcrichton added beta-nominated Nominated for backporting to the compiler in the beta channel. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 27, 2015
@steveklabnik
Copy link
Member

Does this let Cargo build on stable? IIRC, it was only missing some FS labels.

I'm 👍 to backporting this to beta because it's 1.1. If it were 1.2, I'd say just wait.

@alexcrichton
Copy link
Member Author

It does indeed! Cargo also needs the catch_panic feature to build 100% on stable, but it can just not use that feature of git2-rs for now and have panics turn into aborts (Cargo rarely panics anyway)

@retep998
Copy link
Member

With this winapi will be able to drop its dependence on libc!

@alexcrichton alexcrichton force-pushed the stabilize-fs-features branch 3 times, most recently from 1bd9143 to ab0e4d7 Compare May 28, 2015 21:14
@BurntSushi
Copy link
Member

Yay! I think this also lets xsv compile on Rust stable too.

👍

@alexcrichton alexcrichton added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 2, 2015
@alexcrichton
Copy link
Member Author

This PR is now entering the final comment period for stabilizing these APIs!

@sfackler
Copy link
Member

sfackler commented Jun 4, 2015

Should os::windows::fs::MetadataExt::{creation_time, last_access_time, last_write_time} be left unstable until a suitable time type exists, or perhaps renamed in the same style as wait_timeout_ms?

@alexcrichton
Copy link
Member Author

I personally view os::*::fs::MetadataExt as accessors of the raw underlying fields, so even if we had an abstraction for time I'd probably still opt to not return the abstraction but rather the u64 itself. There definitely does seem to be a use case, however, for a middle layer of abstraction sometimes, and this may not be the best way to draw the line in the end.

@sfackler
Copy link
Member

sfackler commented Jun 4, 2015

Works for me.

@bors
Copy link
Contributor

bors commented Jun 5, 2015

☔ The latest upstream changes (presumably #26028) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -663,15 +623,17 @@ impl Permissions {
}
}

#[unstable(feature = "file_type", reason = "recently added API")]
impl FileType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably missed the discussion but why is this not an (extensible) enum? This seems like a really good candidate. Yes, some of the variants won't make sense on certain platforms but I don't think that's really an issue (we'll just have extra variants) and being able to match on filetype would be really nice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variants:

enum FileType {
    // Common
    Directory, File, Symlink,
    // Windows Only
    ReparsePoint,
    // Unix Only
    CharDev, BlockDev, NamedPipe, Socket,
    #[doc(hidden)]
    __non_exhaustive
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're currently not necessarily considering a hidden/unstable variant to be an "unstable enum", it was mostly just how ErrorKind worked out. The set of file types also varies quite a bit across platforms, and the extra types made somewhat more sense to provide through extension traits instead of via this enum.

Finally, using an opaque structure allows one day accessing the raw bits used to determine the file type which may contain some unknown information.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually makes it easier to write cross-platform code because the coder doesn't need to care about the platform: they just handle all cases on all platforms (when possible).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's somewhat of a fine line between being able to write cross-platform code while still understand what is indeed cross platform. If we were to have an exhaustive enum which is the union of all file types on all platforms, it's suddenly not clear what file types come up on which platforms. Which is to say that it's not clear to me that having a union is indeed more cross platform as you're probably still going to have platform-specific code handling various file types on various platforms.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're currently not necessarily considering a hidden/unstable variant to be an "unstable enum", it was mostly just how ErrorKind worked out.

Given that we're already doing this, I don't see why we shouldn't do it here.

The set of file types also varies quite a bit across platforms, and the extra types made somewhat more sense to provide through extension traits instead of via this enum.

The problem with extension traits is that they force the user to care about the platform. With an enum, I could write:

fn format_filetype(s: &mut String, entry: &DirEntry) {
   match entry.file_type() {
        File => s.push('f'),
        NamedPipe => s.push('p'),
        ReparsePoint => s.push('r'),
        Other => s.push('?'),
        // ...
   }
}

With extension traits, I need one function per platform (each one importing a different extension trait). Extension traits are useful when some set of functionality is nonsensical on a platform but in this case, the extra file types make sense, they just aren't used.

Finally, using an opaque structure allows one day accessing the raw bits used to determine the file type which may contain some unknown information.

This unknown information is stored in the Metadata structure. Are you worried about unknown file types or something? Pretty much by definition, a file should only have one type so an enum should always be able to describe it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not re-loading before posting... For now, I feel that the enum is small enough that this isn't really an issue however, I can see this becoming an issue in the future as more platforms are added and this enum grows. So I see your point.

@alexcrichton
Copy link
Member Author

triage: beta-accepted

@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 9, 2015
@alexcrichton
Copy link
Member Author

re-r? @aturon

I've rebased and updated this PR to include a snippet in RELEASES.md to talk about the new features.

pub type c_float = f32;
pub type c_double = f64;
#[cfg(target_arch = "aarch64")]
#[stable(feature = "raw_os", since = "1.0.0")] pub type c_char = u8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, shouldn't all these be stable for 1.1.0 and not 1.0.0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah oops, thanks!

This commit stabilizes the following APIs, slating them all to be cherry-picked
into the 1.1 release.

* fs::FileType (and transitively the derived trait implementations)
* fs::Metadata::file_type
* fs::FileType::is_dir
* fs::FileType::is_file
* fs::FileType::is_symlink
* fs::DirEntry::metadata
* fs::DirEntry::file_type
* fs::DirEntry::file_name
* fs::set_permissions
* fs::symlink_metadata
* os::raw::{self, *}
* os::{android, bitrig, linux, ...}::raw::{self, *}
* os::{android, bitrig, linux, ...}::fs::MetadataExt
* os::{android, bitrig, linux, ...}::fs::MetadataExt::as_raw_stat
* os::unix::fs::PermissionsExt
* os::unix::fs::PermissionsExt::mode
* os::unix::fs::PermissionsExt::set_mode
* os::unix::fs::PermissionsExt::from_mode
* os::unix::fs::OpenOptionsExt
* os::unix::fs::OpenOptionsExt::mode
* os::unix::fs::DirEntryExt
* os::unix::fs::DirEntryExt::ino
* os::windows::fs::MetadataExt
* os::windows::fs::MetadataExt::file_attributes
* os::windows::fs::MetadataExt::creation_time
* os::windows::fs::MetadataExt::last_access_time
* os::windows::fs::MetadataExt::last_write_time
* os::windows::fs::MetadataExt::file_size

The `os::unix::fs::Metadata` structure was also removed entirely, moving all of
its associated methods into the `os::unix::fs::MetadataExt` trait instead. The
methods are all marked as `#[stable]` still.

As some minor cleanup, some deprecated and unstable fs apis were also removed:

* File::path
* Metadata::accessed
* Metadata::modified

Features that were explicitly left unstable include:

* fs::WalkDir - the semantics of this were not considered in the recent fs
  expansion RFC.
* fs::DirBuilder - it's still not 100% clear if the naming is right here and if
  the set of functionality exposed is appropriate.
* fs::canonicalize - the implementation on Windows here is specifically in
  question as it always returns a verbatim path. Additionally the Unix
  implementation is susceptible to buffer overflows on long paths unfortunately.
* fs::PathExt - as this is just a convenience trait, it is not stabilized at
  this time.
* fs::set_file_times - this funciton is still waiting on a time abstraction.
@aturon
Copy link
Member

aturon commented Jun 12, 2015

This has gone through FCP and been reviewed by the core team, and we're ready to land the stabilization!

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 12, 2015

📌 Commit ec68c4a has been approved by aturon

@bors
Copy link
Contributor

bors commented Jun 12, 2015

⌛ Testing commit ec68c4a with merge 50ab23d...

bors added a commit that referenced this pull request Jun 12, 2015
This commit stabilizes the following APIs, slating them all to be cherry-picked
into the 1.1 release.

* fs::FileType (and transitively the derived trait implementations)
* fs::Metadata::file_type
* fs::FileType::is_dir
* fs::FileType::is_file
* fs::FileType::is_symlink
* fs::DirEntry::metadata
* fs::DirEntry::file_type
* fs::DirEntry::file_name
* fs::set_permissions
* fs::symlink_metadata
* os::raw::{self, *}
* os::{android, bitrig, linux, ...}::raw::{self, *}
* os::{android, bitrig, linux, ...}::fs::MetadataExt
* os::{android, bitrig, linux, ...}::fs::MetadataExt::as_raw_stat
* os::unix::fs::PermissionsExt
* os::unix::fs::PermissionsExt::mode
* os::unix::fs::PermissionsExt::set_mode
* os::unix::fs::PermissionsExt::from_mode
* os::unix::fs::OpenOptionsExt
* os::unix::fs::OpenOptionsExt::mode
* os::unix::fs::DirEntryExt
* os::unix::fs::DirEntryExt::ino
* os::windows::fs::MetadataExt
* os::windows::fs::MetadataExt::file_attributes
* os::windows::fs::MetadataExt::creation_time
* os::windows::fs::MetadataExt::last_access_time
* os::windows::fs::MetadataExt::last_write_time
* os::windows::fs::MetadataExt::file_size

The `os::unix::fs::Metadata` structure was also removed entirely, moving all of
its associated methods into the `os::unix::fs::MetadataExt` trait instead. The
methods are all marked as `#[stable]` still.

As some minor cleanup, some deprecated and unstable fs apis were also removed:

* File::path
* Metadata::accessed
* Metadata::modified

Features that were explicitly left unstable include:

* fs::WalkDir - the semantics of this were not considered in the recent fs
  expansion RFC.
* fs::DirBuilder - it's still not 100% clear if the naming is right here and if
  the set of functionality exposed is appropriate.
* fs::canonicalize - the implementation on Windows here is specifically in
  question as it always returns a verbatim path. Additionally the Unix
  implementation is susceptible to buffer overflows on long paths unfortunately.
* fs::PathExt - as this is just a convenience trait, it is not stabilized at
  this time.
* fs::set_file_times - this funciton is still waiting on a time abstraction.
@bors bors merged commit ec68c4a into rust-lang:master Jun 12, 2015
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 17, 2015
@alexcrichton alexcrichton deleted the stabilize-fs-features branch July 10, 2015 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants